feat(flags): support mixed targeting in local evaluation#523
feat(flags): support mixed targeting in local evaluation#523patricio-posthog merged 9 commits intomainfrom
Conversation
posthog-python Compliance ReportDate: 2026-04-24 14:14:17 UTC ✅ All Tests Passed!30/30 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 1/1 tests passed View Details
|
Prompt To Fix All With AIThis is a comment left during a code review.
Path: posthog/feature_flags.py
Line: 326-331
Comment:
**Redundant guard variable (`has_condition_aggregation`)**
`has_condition_aggregation` is always `True` whenever `condition_aggregation != flag_aggregation` is `True`. When the key is absent from `condition`, `.get("aggregation_group_type_index", flag_aggregation)` returns `flag_aggregation`, so `condition_aggregation == flag_aggregation` is guaranteed — the `!=` test is already `False` before `has_condition_aggregation` is checked. The extra variable adds complexity without changing behavior (simplicity rule 4: no superfluous parts).
```suggestion
condition_aggregation = condition.get(
"aggregation_group_type_index", flag_aggregation
)
if condition_aggregation != flag_aggregation:
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 3829-3867
Comment:
**Duplicate test — identical to `test_mixed_targeting_person_condition_matches`**
`test_mixed_targeting_group_without_groups_skips_to_person` is byte-for-byte the same flag definition, the same `get_feature_flag` call (no `groups` arg, `person_properties={"email": "test@example.com"}`), and the same assertions as `test_mixed_targeting_person_condition_matches` at line 3710. One of them should be removed; the surviving test can carry the inline comment explaining the skip behaviour. This violates the OnceAndOnlyOnce simplicity rule.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 3869-3896
Comment:
**Unused mock setup and missing call-count assertion**
`patch_flags.return_value = {"featureFlags": {"mixed-flag": "from-api"}}` is dead code here — since all group conditions skip locally and `is_inconclusive` stays `False`, the function returns `False` without ever calling `flags`, so the mock return value is never read. Every other new test also asserts `patch_flags.call_count == 0` to prove no network fallback occurred; this test omits that guard, leaving the no-fallback behaviour unverified.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 3709-3940
Comment:
**Prefer parameterised tests**
Five of the six new tests use an identical flag definition (two conditions: one group, one person). Per the project's coding standards, parameterised tests are preferred. Factoring the common flag fixture out and using `@parameterized.expand` (or a data-driven equivalent) would reduce the duplication significantly and make it straightforward to add further inputs in future.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "chore: ruff format" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Enables local evaluation of feature flags that use mixed person + group targeting by resolving aggregation context (properties + bucketing value) per condition rather than only at the flag level.
Changes:
- Update
match_feature_flag_propertiesto support per-conditionaggregation_group_type_index, selecting the correct properties and bucketing value per condition. - Pass group context (
group_type_mapping,groups,group_properties) fromClient._compute_flag_locallyintomatch_feature_flag_properties. - Add new unit tests covering mixed targeting scenarios and add a changeset entry for release notes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
posthog/feature_flags.py |
Implements per-condition aggregation handling for mixed local flag evaluation (properties + bucketing selection). |
posthog/client.py |
Ensures local evaluation passes group context needed for mixed targeting conditions. |
posthog/test/test_feature_flags.py |
Adds test coverage for mixed person/group targeting behavior and bucketing correctness. |
.sampo/changesets/mixed-targeting-local-eval.md |
Documents the patch change for release automation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dmarticus
left a comment
dmarticus
left a comment
There was a problem hiding this comment.
Review
Nice fix — scope is tight and the test coverage lines up well with the behaviors I care about (per-condition bucketing identity, skip-vs-inconclusive split, no-groups-passed). Approving.
I left three non-blocking inline comments on specific lines. One more observation that didn't fit inline:
- Cohort properties on group-aggregated conditions (follow-up, out of scope). A group-aggregated condition containing a cohort property would pass group props into
match_cohort, but cohorts are person-defined. Today the UI probably doesn't let you build this shape, but if it ever does we'd evaluate the cohort against the wrong property bag. Worth a follow-up issue if you agree.
Pastable prompt to address these comments
If you want to knock these out in one pass, feel free to paste this into your editor:
In
posthog/feature_flags.py, insidematch_feature_flag_properties:
When a condition's
aggregation_group_type_indexresolves to an unknown group (not ingroup_type_mapping, or missing fromgroups), add alog.debug(...)beforecontinueso misconfigured mixed flags are diagnosable. Mirror the message style used in_compute_flag_locally's pure-group-flag path, but at debug level (not warning) since per-condition skips are expected.Add a one-line comment above the
if condition_aggregation != flag_aggregation:block noting that the mixed-override path assumes flag-level aggregation isNone; the inverse shape (flag-level set, condition-levelNone) isn't supported by the current caller and would silently pass group props as person props. Documentation only — no code change.In
.sampo/changesets/mixed-targeting-local-eval.md, tighten the changeset body to a single user-facing sentence, e.g.Support mixed user+group targeting in local flag evaluation.The implementation detail in the current second sentence isn't something end users need.(Optional follow-up, separate PR.) Cohort properties evaluated inside a group-aggregated condition will be matched against group props, not person props. File an issue or add a TODO if you agree it's worth tracking.
No new tests required for any of these.
Created with PostHog Code
Problem
Flags using mixed user+group targeting (the beta feature) cannot be evaluated locally by the SDK. The SDK reads
aggregation_group_type_indexat the flag level only. For mixed flags this isNone, so all conditions are evaluated as person targeting, causing group conditions to fail and fall back to an HTTP call.This breaks customers using flags in environments that cannot make HTTP calls (e.g., Temporal workflows).
Changes
Updated
match_feature_flag_propertiesto resolve aggregation per condition when a condition explicitly sets its ownaggregation_group_type_indexthat differs from the flag level. Each condition uses the correct properties and bucketing value for its aggregation type. Backwards compatible with existing pure person and pure group flags.How did you test this code?
All 115 existing tests pass unchanged. Added 6 new tests covering mixed targeting scenarios: person match, group match, no match, group skips to person, no groups passed, and correct bucketing per condition.